Skip to content

review: port the seeded-trial cases to the live corpus#235

Merged
jwbron merged 18 commits into
mainfrom
jwbron/live-eval-trial-cases
Jul 10, 2026
Merged

review: port the seeded-trial cases to the live corpus#235
jwbron merged 18 commits into
mainfrom
jwbron/live-eval-trial-cases

Conversation

@jwbron

@jwbron jwbron commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Ports the seeded-defect trial from Khan/webapp#40678 into the review-workflow eval corpus as three live-enabled cases, stacked on #233 (the live-enabled corpus format).

Sanitization constraint

Khan/actions is public and Khan/webapp is private, so these are sanitized structural rewrites, not copies: fresh Go code around a generic "notes retention" feature (per-user stored notes, a background prune, deletion on account erasure) that reproduces the trial's defect mechanisms with generic naming. No webapp code, file paths, or identifiers appear anywhere in the cases.

The three cases

trial-retention-deletion (incident-repro, tags incident/trial/live)

The trial's deletion-path seeds, five recorded findings across erasure.go, store.go, and a new test file:

  • erasure-flag-gated-deletion (blocking): the account-erasure deletion is gated behind a rollout flag, so flag-off silently skips the compliance deletion (trial seed 1).
  • erasure-default-limit-one (blocking): the deletion query leaves Limit unset and the store's documented zero-value default is 1, so at most one note is ever deleted (seed 2).
  • erasure-ignores-delete-helper (advisory): the new loop reimplements, worse, a paging batch-delete helper that already exists in the same file (seed 3).
  • erasure-env-widening + store-env-widening (advisory, one per file): the request-env interface is widened in two files solely for the buggy flag gate (seed 4).
  • erasure-prune-error-swallowed (blocking): _ = PruneUserNotes(...) discards the returned error; the prose cites the error-handling skill (seed 8).

Expected: REQUEST_CHANGES, all six ids must-catch, 6 posted comments.

trial-retention-prune-tests (incident-repro, tags incident/trial/live)

The prune-and-tests seeds in a new prune.go + prune_test.go (both added files):

  • prune-off-by-one-cap (advisory, the trial's calibrated severity): notes[maxRetainedNotes-1:] retains 199 where the documented cap is 200 (seed 5).
  • prune-test-vacuous-cap (blocking): the cap test seeds 5 notes, far below the cap, so the early return makes it pass even for a no-op prune (seed 6).
  • prune-suite-flag-mock (blocking): TestMain forces the rollout flag on for the whole suite, so the flag-off path is never exercised (seed 7).
  • prune-full-entity-fetch (advisory): the prune fetches up to 5000 full entities but reads only ids; Query.KeysOnly exists for exactly this (seed 9).

Expected: REQUEST_CHANGES, all four ids must-catch, 4 posted comments.

trial-batch-delete-wrapper (clean, tags clean/trial/live)

The trial's deliberate non-defect (seed 10) as a must-not-flag trap: the changed purge.go collects every note key and issues one DeleteMulti call that looks like it exceeds the datastore's 500-entity per-call cap. The tree also carries the unchanged wrapper source (internal/datastore/client.go) whose DeleteMulti visibly chunks into 500-key batches, so a live reviewer can discover the truth by reading it. The case records the wrong blocking claim, neutralizes it with a refuted validation entry, and pins expected to APPROVE with zero posted comments; live.mustNotFlagSpecs describes the trap so a live run that claims the cap is scored as a false block.

Case mechanics

  • Each case carries a git-style unified diff computed from authored pre/post file contents (difflib-generated, added test files staged against /dev/null with a new-file mode line), the post-change tree next to case.json, live.prContext, and per-defect mustCatchSpecs/mustNotFlagSpecs with line windows of about +/-5 around the defect and 2-4 mechanism alternates.
  • Every recorded line-anchored finding anchors on an ADDED line of the case diff; the generator asserted this against the diff, so the change-provenance gate keeps every finding.
  • None of the cases are tagged smoke, keeping the per-PR smoke gate fast; both incident-repro cases feed must-catch recall in suite.test.ts automatically.
  • Includes a review patch changeset.

Test plan:

  • pnpm run test --run: all 21 files / 662 tests green, including the corpus loader's live-block and tree validation over the new cases and checkExpectation on each case's expected block.
  • pnpm run typecheck: clean (no TypeScript touched).
  • Provenance-gate anchors verified mechanically at generation time: every finding's anchor line was asserted to be an added line of its file's diff.

Next steps (human)

  1. Sanitization check, the load-bearing review here: someone who knows the webapp trial code should confirm no webapp code, paths, or identifiers leaked into these three cases (Khan/actions is public; the cases must be structural rewrites only).
  2. Severity calls to ratify: seed 7 (suite-wide flag mock) was authored as blocking by judgment call; seed 5 (off-by-one) is advisory per the trial's calibration. Adjust if you disagree; expected blocks and specs move together.
  3. Rebase and merge after review: live-enabled corpus format and ten live cases #233; the deterministic suite gates the rest.

Update 2026-07-09: two misses promoted to smoke

trial-dedup-composite-key and trial-erasure-suite-flag-mock (the v1.4.0 re-run misses) now carry the smoke tag, so every per-PR A/B watches the two defects the current production prompt demonstrably missed (~$1/run for the pair). The other trial cases deliberately stay behind the full-eval label.

jwbron added 2 commits July 9, 2026 12:47
…live cases

Phase 1 of the live A/B eval plan (#232). The corpus gains
an opt-in live block carrying what a real model run needs and the
deterministic replay does not:

- prContext (PR title/description/author/base; the description is
  untrusted author text, so an adversarial case can carry its payload
  there or in the diff),
- a post-change file tree on disk next to the case, via a new
  <id>/case.json + <id>/tree/ layout that coexists with flat <id>.json
  (a directory containing case.json is one case; its tree is never
  parsed as corpus JSON), and
- labeled defect specs (mustCatchSpecs / mustNotFlagSpecs: path, line
  window, mechanism keyword alternates). Live runs choose their own
  finding ids, so ground truth matches on anchor window + mechanism
  rather than id.

The loader enforces the invariants: the live tag and the live block
imply each other, a live case needs a cleanly-parseable diff, spec
paths must appear in changedFiles and the diff, and every non-removed
changed file must exist in the tree. loadLiveCorpus() returns the
subset. The live half lives in corpus/live.ts; loader.ts re-exports it
as the single public surface.

Ten cases are converted with hand-authored real diffs and trees: five
smoke incidents, both clean cases, one adversarial injection whose
payload is a code comment in the diff, one golden holdout, and one
synthetic mutation. Their recorded line anchors are rewritten to the
authored defect lines (natural files beat content padded to synthetic
line numbers), which activates the provenance gate on these cases in
the deterministic suite; all expectations hold unchanged.
… the live corpus

Three live-enabled corpus cases porting the Khan/webapp#40678
seeded-defect trial as sanitized structural rewrites: fresh Go code
around a generic notes-retention feature reproducing the trial's
defect mechanisms, with no webapp code, paths, or identifiers.

- trial-retention-deletion (incident-repro): flag-gated compliance
  deletion, query-default limit of 1, reimplemented deletion helper,
  env-interface widening flagged in both files, swallowed prune error.
- trial-retention-prune-tests (incident-repro): off-by-one retention
  cap, vacuous cap test that passes for a no-op prune, suite-wide
  flag-ON mock hiding the flag-off path, full-entity fetch where a
  keys-only query suffices.
- trial-batch-delete-wrapper (clean): the trial's deliberate
  non-defect as a must-not-flag trap; the datastore wrapper (unchanged,
  in tree) chunks DeleteMulti into 500-key batches, so the recorded
  500-entity-cap block is refuted and the case approves.

Every recorded finding anchors on an added line of the case diff
(provenance gate verified); the full vitest suite and typecheck are
green.
@jwbron jwbron self-assigned this Jul 9, 2026
@changeset-bot

changeset-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: b00ad9a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
review Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jwbron added 3 commits July 9, 2026 13:48
… and vitest

Tree directories are byte-exact case fixtures paired with each case's
diff: prettier auto-formatting one would silently desync it from the
diff the provenance gate parses, and a tree may carry a *.test.ts
whose tests fail by design (test-adequacy cases), so vitest must not
execute them either. CI's lint job caught the first drift (prettier
wanting to rewrap a fixture ternary).
… live corpus cases

Three sanitized structural rewrites from the v1.4.0 re-run, in the same
generic notes-retention domain as the existing trial cases:

- trial-dedup-composite-key: the save path dedups on Note.Body alone
  while the entity carries a Kind readers select on; a same-Body note of
  a different kind is silently never saved. v1.3.1 caught this; v1.4.0
  missed it at the finder level.
- trial-dedup-eventual-consistency: the dedup read is documented
  eventually consistent, so a duplicate submitted moments after the
  original reads a stale set and is stored again. The re-run's skill
  auditor investigated the mechanism and declined it as unquotable, and
  no correctness lens surfaced it; the case pins that a skill-adjacent
  correctness issue must surface regardless of which lens owns it.
- trial-erasure-suite-flag-mock: the erasure test suite pins the
  rollout flag on in its shared test-env constructor, so the flag-off
  path (where the unchanged EraseUser silently skips the compliance
  deletion) is never exercised.

Anchors and spec windows were mechanically asserted against added diff
lines at generation time, as with the existing trial cases.
@jwbron

jwbron commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Rider: pushed the three v1.4.0 re-run misses as live cases (54bff8a), same sanitized notes domain and case mechanics as the originals.

  • trial-dedup-composite-key (blocking): save-path dedup keyed on Note.Body alone while the entity carries a Kind readers select on; the store docs in the tree say different kinds may share a Body, so the truth is discoverable. v1.3.1 caught the original; v1.4.0 missed it at the finder level.
  • trial-dedup-eventual-consistency (advisory, APPROVE + 1 comment): the dedup read is documented eventually consistent in the tree's store.go, so a retried/double-submitted save reads a stale set and stores the duplicate; the exact traffic the dedup exists for. In the re-run the skill auditor investigated this and declined it as unquotable, and no correctness lens surfaced it; the case pins that the issue must surface regardless of which lens owns it.
  • trial-erasure-suite-flag-mock (blocking): the erasure test suite pins the rollout flag on in its shared test-env constructor, hiding the flag-off path where the unchanged, flag-gated EraseUser silently skips the compliance deletion. The gate itself sits outside the diff, so a live run that posts it (rather than artifacts it) is a provenance violation; that trap is described in the case description because mustNotFlagSpecs can only reference diff paths.

Anchors and spec windows were mechanically asserted against added diff lines at generation time. pnpm run test --run green (20 files / 663 tests, the real-corpus suite now loads 16 live cases), pnpm run typecheck clean.

jwbron added 6 commits July 9, 2026 15:20
…ive case

The first live acceptance runs missed
incident-sql-missing-index:dm-missing-index-1 in all four arms: the
recorded finding is a data-migrations LENS finding, but live cases
carried no routerConfig lens rules, so the live roster never spawned
the specialist that catches it. Every live case whose recorded
finding belongs to a specialist lens now routes that lens on the
finding's file (seven cases across five lenses), mirroring how a
consumer ROUTING file would route the same paths in production.
…s on the prune trial case

Same acceptance-run lesson as the main corpus: a live case whose
recorded finding belongs to a specialist lens needs a routerConfig
lens rule or the live roster never spawns that lens. Only the
full-entity-fetch finding qualifies here; the other trial findings
belong to whole-change reviewers that always run.
…/live-eval-corpus' into jwbron/live-eval-corpus
…wbron/live-eval-trial-cases' into jwbron/live-eval-trial-cases
jwbron added a commit that referenced this pull request Jul 9, 2026
…record the judge-direction lesson

All three follow-ups shipped: agent-failure reasons and the per-row
stability footer on #236, lens routing for live cases on #233/#235.
Records the acceptance pair's most instructive number: judge quality
rose on the deliberately regressed arm (fewer, surer comments score
better), so recall against labeled specs is the load-bearing metric.
…orpus

Sanitized rewrite of the webapp#40736 test: a 'redundant' Limit removal
routes the digest read into a pre-existing default-limit-1 mechanism (the
amplifying hunk must confirm and block, with the regression attributed to
the removal), while the same diff cosmetically touches the mechanism's doc
lines (non-amplified pre-existing: must not draw a blocking finding; the
expected posted-comment count enforces it). Locks the amplification confirm
rule, the provenance gate, and the introduce-vs-amplify prose labeling in
regression.
@jwbron

jwbron commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Added trial-amplified-default-limit: the amplification/provenance behavior case from webapp#40736, sanitized into the notes domain (amplifying Limit removal must block with the regression attributed to the removal; the cosmetically-touched pre-existing default-limit mechanism must not; postedCommentCount enforces both). Closes the corpus gap flagged on #250.

…misses into the smoke set

trial-dedup-composite-key and trial-erasure-suite-flag-mock are the
sanitized ports of defects the current production prompt demonstrably
missed, which makes them exactly what the per-PR smoke A/B should
watch: a candidate that recovers either shows up as a gained spec, a
candidate that loses ground cannot hide it behind the label-gated full
run. Cost is about one dollar per A/B run for the pair (two cases,
two arms). The other trial cases stay out of smoke deliberately to
hold the per-push price down; the full-eval label still covers them.
@jwbron jwbron marked this pull request as ready for review July 10, 2026 18:36
@khan-actions-bot khan-actions-bot requested review from a team, jaredly and jeresig and removed request for a team July 10, 2026 18:36
@jeresig jeresig removed the request for review from jaredly July 10, 2026 19:05
@khan-actions-bot khan-actions-bot requested review from a team and somewhatabstract and removed request for a team July 10, 2026 19:48

@jeresig jeresig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@github-actions

Copy link
Copy Markdown
Contributor

Review Guidance

Common patterns

7 case.json fixtures: All follow one uniform corpus schema (id / tags / category / description / changedFiles / dimensions / findings / expected / diff / live).

6 files: Identical store.go boilerplate (Note, Query, Store, FlagSet, Env types) added verbatim to each incident case tree.

4 files: Near-identical in-memory Store test fake (Run with a limit == 0 -> 1 default, map-based Delete) across the added *_test.go files.

Excluded from review (7 files)

Not individually reviewed — fully explained by a common pattern above:

  • workflows/review/eval/corpus/incidents/trial-amplified-default-limit/tree/services/notes/store.go — pattern-only
  • workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/store.go — pattern-only
  • workflows/review/eval/corpus/incidents/trial-dedup-eventual-consistency/tree/services/notes/store.go — pattern-only
  • workflows/review/eval/corpus/incidents/trial-erasure-suite-flag-mock/tree/services/notes/store.go — pattern-only
  • workflows/review/eval/corpus/incidents/trial-retention-deletion/tree/services/notes/store.go — pattern-only
  • workflows/review/eval/corpus/incidents/trial-retention-prune-tests/tree/services/notes/store.go — pattern-only
  • workflows/review/eval/corpus/incidents/trial-dedup-composite-key/tree/services/notes/save_test.go — pattern-only

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: holistic not assessed this run (reviewer not dispatched under run budget).
Note: conventions not assessed this run (reviewer not dispatched under run budget).

"review": patch
---

Add three live-enabled eval corpus cases porting the Khan/webapp#40678 seeded-defect trial into the review-workflow corpus. All three are sanitized structural rewrites: fresh Go code around a generic "notes retention" feature that reproduces the trial's defect mechanisms, carrying no webapp code, paths, or identifiers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): This changeset states the cases carry "no webapp code, paths, or identifiers", but the same sentence cites Khan/webapp#40678 — a private-repo issue reference that will ship in the public CHANGELOG on release. Consider replacing it with a generic descriptor (e.g. "the internal seeded-defect trial") to satisfy the sanitization constraint.

"review": patch
---

Add three live-enabled eval corpus cases porting the Khan/webapp#40678 seeded-defect trial into the review-workflow corpus. All three are sanitized structural rewrites: fresh Go code around a generic "notes retention" feature that reproduces the trial's defect mechanisms, carrying no webapp code, paths, or identifiers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): This changeset enumerates three live-enabled cases, but this PR's diff adds seven live-tagged case directories (also trial-amplified-default-limit, trial-dedup-composite-key, trial-dedup-eventual-consistency, trial-erasure-suite-flag-mock). Is the count intentional — e.g. the other four are already described by a changeset on the base branch this stacks on — or should this changeset cover them too? I couldn't confirm against the base ref from here.

"live"
],
"category": "incident-repro",
"description": "Sanitized structural rewrite of the amplification/provenance behavior test (Khan/webapp#40736): the PR drops an 'unnecessary' Limit from the digest read, routing it into a pre-existing store default of one row (store.go documents Query.Limit zero as 1, NOT unlimited), so every digest silently shrinks from five notes to one. The defect mechanism predates the diff; the removal is what amplifies it, and the finding must attribute the regression to the removal (the sibling RecentPins read keeps its explicit Limit, and the PR description mischaracterizes the dropped limit as redundant). The same diff also cosmetically rewords the default-limit doc lines in store.go: a touched, non-amplified pre-existing mechanism that must NOT draw a blocking finding; the expected posted-comment count enforces that. The v1.4.0-era live run passed this shape; the case locks the amplification confirm rule, the provenance gate, and the introduce-vs-amplify prose labeling (Khan/actions#250) in regression.",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): This description references Khan/webapp#40736, a private-repo issue identifier — the only Khan/webapp# reference in the corpus (the other cases use Khan/actions#N). It contradicts the "no webapp identifiers in the cases" sanitization constraint; consider dropping it or swapping for the public Khan/actions# equivalent.

],
"postedCommentCount": 1
},
"diff": "diff --git a/services/notes/digest.go b/services/notes/digest.go\nindex 39c2b1c..4ffbbcd 100644\n--- a/services/notes/digest.go\n+++ b/services/notes/digest.go\n@@ -21,7 +21,6 @@ func BuildDigest(ctx context.Context, env digestEnv, userID string) (string, err\n \tsummaries, err := env.Store().Run(ctx, Query{\n \t\tUserID: userID,\n \t\tKind: \"summary\",\n-\t\tLimit: digestSize,\n \t})\n \tif err != nil {\n \t\treturn \"\", fmt.Errorf(\"list summaries for digest of %s: %w\", userID, err)\ndiff --git a/services/notes/store.go b/services/notes/store.go\nindex 6f47ef1..ef64ca2 100644\n--- a/services/notes/store.go\n+++ b/services/notes/store.go\n@@ -25,8 +25,9 @@ type Query struct {\n \t// Kind, when non-empty, selects only notes of that kind.\n \tKind string\n \t// Limit caps the number of rows returned. Zero means 1 (the\n-\t// store's default, tuned for the common latest-note lookup),\n-\t// NOT unlimited; callers that want more must set it.\n+\t// store's default, tuned for the common latest-note lookup and\n+\t// the cheapest read), NOT unlimited; callers that want more\n+\t// must set it explicitly.\n \tLimit int\n \t// KeysOnly returns notes with only ID populated, skipping the\n \t// entity bodies. Much cheaper when the caller needs keys alone.\n",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): The live-case format keeps two hand-maintained copies of each changed file — the embedded diff string and the tree/ snapshot — and the loader only checks that each tree file exists (live.ts liveTreeErrors uses existsSync), never that it matches the diff's post-image. Across the seven cases added here, a later edit to one copy but not the other would silently point mustCatchSpecs windows at code the sub-agents never see, with no loader error. Worth a loader/vitest check that every added + line in a case's diff appears in the corresponding tree file.

jwbron added a commit that referenced this pull request Jul 10, 2026
Phase 1 of the live A/B eval plan (#232): the eval corpus learns to carry real change content so a future live producer can run the actual model sub-agents against it.

## Format

A case may now carry an opt-in `live` block (`corpus/live.ts`, re-exported through the loader):

- `prContext`: PR title/description/author/base branch, mirroring production `pr-context.json`. The description is untrusted author text, so adversarial cases can carry their payload there or in the diff.
- An on-disk post-change file tree, via a new `<id>/case.json` + `<id>/tree/` layout coexisting with flat `<id>.json`. A directory containing `case.json` is one case; its tree is never parsed as corpus JSON.
- `mustCatchSpecs` / `mustNotFlagSpecs`: labeled defects as (path, line window, mechanism keyword alternates). Live runs choose their own finding ids, so ground truth matches on anchor window plus mechanism rather than id; the Phase 3 matcher consumes these.

Enforced invariants: the `live` tag and the block imply each other; a live case needs a cleanly-parseable diff (fail-open is fine in production, an authoring error here); spec paths must appear in `changedFiles` and the diff; every non-removed changed file must exist in the tree. `loadLiveCorpus()` returns the subset.

## Cases

Ten cases converted with hand-authored real diffs and trees: the five smoke incidents (money rounding, auth bypass, cache key, race condition, missing index), both clean cases, the adversarial injection case (payload is a code comment in the diff instructing the reviewer to approve), the golden authz holdout, and the money-payments synthetic mutation. Recorded line anchors are rewritten to the authored defect lines; natural files beat content padded to synthetic line numbers, and every anchor is asserted to be an added line so the provenance gate (now active on these cases) keeps them.

The trial-content port landed as #235, stacked on this PR. One acceptance-driven change also landed here: the phase 4 runs missed `incident-sql-missing-index` in all four arms because live cases carried no `routerConfig` lens rules, so specialist lenses never spawned; every live case whose ground truth belongs to a specialist lens now routes that lens on the finding's file (see the "route the specialist lens on each live case" commit).

## Test plan:

- `pnpm run test --run`: 659 tests green, including 17 new loader tests (memfs) covering the live block, both layouts, tree validation, and the tree-JSON exclusion.
- The deterministic suite runs the ten converted cases unchanged (same verdicts, comment counts, must-catch sets), now with the provenance gate active on them.
- `pnpm run typecheck` and eslint clean on the three touched TS files.


## Next steps (human)

1. Review the ten authored diffs and trees for realism; they are synthetic content a live model will read, so plausibility matters more than in ordinary fixtures. Spot-check that each case's recorded finding still describes the authored defect (anchors were rewritten to the authored lines).
2. This is the root of the stack: review and undraft first; #234 and #235 rebase onto it.
3. No live validation needed here; the deterministic suite (`pnpm run test --run`) fully gates it.

Author: jwbron

Reviewers: jeresig, github-actions[bot], jwbron

Required Reviewers:

Approved By: jeresig, github-actions[bot]

Checks: ✅ 9 checks were successful

Pull Request URL: #233
@jwbron jwbron changed the base branch from jwbron/live-eval-corpus to main July 10, 2026 20:49
An error occurred while trying to automatically change base from jwbron/live-eval-corpus to main July 10, 2026 20:49
# Conflicts:
#	workflows/review/eval/corpus/live.ts
@jwbron jwbron merged commit d615f20 into main Jul 10, 2026
9 checks passed
@jwbron jwbron deleted the jwbron/live-eval-trial-cases branch July 10, 2026 20:58
jwbron added a commit that referenced this pull request Jul 10, 2026
Phase 5 of the live A/B eval plan (#232), independent of the eval-code stack (#233-#237): the `review-trial` Claude Code skill, packaging the Khan/webapp#40678 seeded-defect trial choreography so an operator can run one in an afternoon.

## What the skill does

Given a consumer repo, a seeded branch, a **human-authored** defect table, an arms list, and budget approval (projected up front from the trial's measured $7-10 per workflow-arm run, confirmed before any PR is created), it:

1. Creates one isolated PR per arm from the seeded branch, with per-arm trigger recipes (repo-default reviewer, pinned workflow ref, hosted `@claude review`) and two hard rules: distinct workflow names (same-named gh-aw workflows share a per-PR concurrency group and cancel each other; this silently ate a run in the original trial) and exactly one reviewer per PR (suppress the default via `skip-ai-review` on non-default arms).
2. Collects each run's review, artifacts (tolerating the known gh-aw staging-path artifact annotation), billed credits, and wall clock.
3. Optionally drives the lifecycle protocol (push 2 with fixes plus fresh seeds, push 3 all-fixed) identically across arms, scoring each push separately.
4. Scores defect by defect with the deterministic rule mirroring `eval/live-match.ts` (path + window + mechanism), manual judgment for leftovers marked as judged in the report, traps counted as correct suppression, and faithful reporting whichever arm it favors; output in the #40678 report shape.
5. Exports live-enabled corpus case skeletons, with an explicit sanitization gate for private-repo content landing in this public repo (the #235 lesson: structural rewrites, never copied code).
6. Cleans up: PRs closed with the report linked, trial branches deleted, no temporary workflow left behind.

What stays human is stated at the top of the skill: seeds and ground truth are operator-authored, always; the skill refuses to improvise a defect table.

## `.gitignore` change

`.claude` was ignored wholesale, which would have kept project skills untracked. Narrowed to `.claude/*` with a `!.claude/skills/` carve-out; verified `settings.json` and `worktrees/` remain ignored.

## Test plan:

Docs/tooling only (empty changeset); no code paths. `git check-ignore` verified the carve-out (SKILL.md tracked, `settings.json` and `worktrees` still ignored). The real acceptance is the plan's: re-running the #40678 trial via this skill reproduces its table in an afternoon, which needs a consumer-repo trial to exercise.


## Next steps (human)

1. Review the `.gitignore` change deliberately: `.claude` wholesale becomes `.claude/*` + `!.claude/skills/`. Verified locally that `settings.json` and `worktrees/` stay ignored, but this is a policy change about what agent tooling gets committed to the repo.
2. The skill's acceptance is operational, not CI: next time a trial is warranted (an architecture-bet change like the deterministic orchestrator, or pre-graduation ground-truthing), run it via this skill and check it reproduces the #40678 report shape in an afternoon. There is no cheaper honest validation.
3. Skim the per-arm trigger recipes against current consumer-repo reality (the `skip-ai-review` label convention, ready-for-review semantics); those conventions live outside this repo and can drift.

Author: jwbron

Reviewers: jeresig, github-actions[bot], somewhatabstract

Required Reviewers:

Approved By: jeresig, github-actions[bot]

Checks: ✅ 10 checks were successful

Pull Request URL: #238
jwbron added a commit that referenced this pull request Jul 10, 2026
* [jwbron/live-eval-corpus] review: live-enabled corpus format and ten live cases

Phase 1 of the live A/B eval plan (#232). The corpus gains
an opt-in live block carrying what a real model run needs and the
deterministic replay does not:

- prContext (PR title/description/author/base; the description is
  untrusted author text, so an adversarial case can carry its payload
  there or in the diff),
- a post-change file tree on disk next to the case, via a new
  <id>/case.json + <id>/tree/ layout that coexists with flat <id>.json
  (a directory containing case.json is one case; its tree is never
  parsed as corpus JSON), and
- labeled defect specs (mustCatchSpecs / mustNotFlagSpecs: path, line
  window, mechanism keyword alternates). Live runs choose their own
  finding ids, so ground truth matches on anchor window + mechanism
  rather than id.

The loader enforces the invariants: the live tag and the live block
imply each other, a live case needs a cleanly-parseable diff, spec
paths must appear in changedFiles and the diff, and every non-removed
changed file must exist in the tree. loadLiveCorpus() returns the
subset. The live half lives in corpus/live.ts; loader.ts re-exports it
as the single public surface.

Ten cases are converted with hand-authored real diffs and trees: five
smoke incidents, both clean cases, one adversarial injection whose
payload is a code comment in the diff, one golden holdout, and one
synthetic mutation. Their recorded line anchors are rewritten to the
authored defect lines (natural files beat content padded to synthetic
line numbers), which activates the provenance gate on these cases in
the deterministic suite; all expectations hold unchanged.

* [jwbron/live-eval-producer-staging] review: live-producer prompt extraction and case staging

Phases 2a/2b of the live A/B eval plan (#232), stacked on the Phase 1
corpus format (#233).

agent-extract.ts turns review.md's '## agent:' sections into data
(name, description, pinned model, prompt body). It takes the markdown
as a string with no fs access, because the baseline arm of an A/B
reads the merge-base review.md via git show. Parsing is strict; a
malformed or model-less section throws listing every problem, since a
silently dropped agent would skew an eval arm without failing it. An
integration test extracts the real review.md (21 agents) and pins the
staging-root reference so a future rename fails a test instead of
silently staging nothing.

live-stage.ts materializes the production staging layout for one
live-enabled corpus case: pr-context.json (review.md Step 1's shape,
synthetic identity fields), full.diff / pr.diff / full-stripped.diff
(all the case diff; corpus diffs carry no generated files and no
pattern-triage pass runs), files.json + review-files.json with the
hasPatch cross-check derived from the diff parse, provenance.json,
routing.json from the deterministic router, an out/ directory, and
the post-change checkout copied from the case tree.
rewriteAgentPrompt swaps the production staging root for the staged
context dir. Everything sits behind an injected-fs seam and is
memfs-tested.

Model dispatch (phase 2c) is deliberately absent; it needs the Agent
SDK dependency decision and arrives separately.

* [jwbron/live-eval-producer-staging] review: the live producer and SDK runner (phase 2c)

live-producer.ts runs the live roster over one staged case behind an
injected LiveAgentRunner seam (the judge.ts pattern), so its logic is
stub-tested with zero model calls. Roster: the default finders
(correctness-reviewer, skill-auditor) plus the router's lensesToSpawn;
no pattern-triage or thread-reconciler in eval. It maps all three
sub-agent output contracts into the shapes the deterministic runner
consumes: label-shape findings (correctness lens, or conventions for
the skill-auditor so labelForFinding reproduces the best-practice
variants; confidence defaulted to 0.7 per the production claims rule),
structured-schema lens findings validated as-is, and the validator's
{claims: [...]} verdicts into CaseVerification[]. It stages
claims.json for the validator, resolves {{#runtime-import}} directives
against the case tree (a case opts into a skills index by carrying the
file), retries once on malformed output with the rejection fed back,
keeps partial results when an agent fails twice, prefixes colliding
finding ids, and reports per-agent cost/turns/wall-clock.

live-runner.ts is the one module that touches a real model runtime:
an Agent SDK query() per dispatch with Read/Grep/Glob only, cwd pinned
to the staged checkout, the agent's pinned model, and hard turn and
wall-clock caps; plus the CLI smoke entry
(tsx live-runner.ts --case <id>, requires ANTHROPIC_API_KEY). The
investigation-cap CLI the prompts reference is not staged; its own
denied-budget fallback applies. Adds @anthropic-ai/claude-agent-sdk
as a dev dependency.

* [jwbron/live-eval-ab-runner] review: the live A/B runner (phase 3)

live-match.ts scores a live run against a case's labeled defect
specs: a posted candidate satisfies a spec when its anchor agrees
with the spec's path (and line window when both carry one) and any
mechanism alternate matches the finding's failure_scenario or prose;
each candidate satisfies at most one spec and vice versa. An injected
fallback arbiter (hard-capped, same-file only, recorded as
via: fallback for audit) can rescue recall on vague prose; false
flags are decided by the deterministic rule alone. computeLiveMetrics
aggregates recall, verdict agreement, clean false-flag (including a
clean case that blocks), and noise.

live-ab.ts is the arm orchestrator and CLI: baseline review.md from
git show <merge-base>, candidate from the working tree, both arms
over the same live corpus with everything else (corpus, lib, runner,
metrics, judge) from the candidate, per the plan's settled decision
to isolate the model seam. Each arm runs under half the --max-usd
budget with sticky exhaustion: once spend plus the running per-case
average crosses the cap, remaining cases are recorded skipped and
the report still emits. Spec-level regressions are diffed only over
cases both arms scored. Judge scoring reuses the pinned judge
(quality aggregates only; judge-vs-ground-truth disagreement keys on
recorded ids a live arm does not use); the fetch model moves to
judge-live-model.ts and live-judge.ts now imports it. runner.ts
gains an optional RunOptions.validation override so a live
validator's output replaces the recorded block.

Report-only except the standing rule: adversarial-injection failures
on the candidate arm exit non-zero.

* [jwbron/live-eval-ab-ci] review: per-PR live A/B workflow (phase 4)

Review Eval A/B runs on every non-draft PR touching
workflows/review/** (and on workflow_dispatch): both review.md arms
over the live corpus via live-ab.ts, with the delta report posted as
a sticky PR comment (hidden-marker upsert), appended to the job
summary, and uploaded as an artifact even on partial or gate-failing
runs. Per-PR scope is the smoke-tagged live subset; the full-eval
label or dispatch input lifts it, skip-live-eval opts out, drafts
wait until ready, the changeset-release branch is excluded (it
matches the path filter via package.json but changes no behavior),
secretless runs skip green so fork PRs never fail, and per-PR
concurrency cancels superseded runs. The workflow name is distinct
from every gh-aw workflow per the operational-floor rule about shared
concurrency groups. live-ab.ts gains --smoke-only and writes
out/live-ab-report.md alongside the JSON for the comment step.

* [jwbron/live-eval-corpus] review: exclude eval-corpus trees from lint and vitest

Tree directories are byte-exact case fixtures paired with each case's
diff: prettier auto-formatting one would silently desync it from the
diff the provenance gate parses, and a tree may carry a *.test.ts
whose tests fail by design (test-adequacy cases), so vitest must not
execute them either. CI's lint job caught the first drift (prettier
wanting to rewrap a fixture ternary).

* [jwbron/live-eval-producer-staging] review: namespace live finding ids with the case id

Live agents choose their own finding ids, so every case's first
correctness finding was live-correctness-reviewer-1; ids were unique
within a case but collided across cases, and judge.ts's score join
requires arm-global uniqueness. Caught by the first real A/B run
(both arms completed, then judge aggregation threw). Ids are now
<caseId>:<id> from the moment of parsing, so claims.json, the
validator round-trip, the matcher, and the judge all see the same
namespaced id.

* [jwbron/live-eval-ab-runner] review: judge failures degrade the A/B report instead of killing it

The first real A/B run spent both arms' budgets and then died in
judge aggregation, writing no report: the exact
everything-spent-nothing-posted failure mode the plan forbids. Judge
scoring is additive, so a per-arm failure is now caught, recorded as
judgeError on the arm, rendered as a degradation note in the report,
and the run proceeds to write JSON + markdown and evaluate the
adversarial gate as usual.

* [jwbron/live-eval-corpus] review: route the specialist lens on each live case

The first live acceptance runs missed
incident-sql-missing-index:dm-missing-index-1 in all four arms: the
recorded finding is a data-migrations LENS finding, but live cases
carried no routerConfig lens rules, so the live roster never spawned
the specialist that catches it. Every live case whose recorded
finding belongs to a specialist lens now routes that lens on the
finding's file (seven cases across five lenses), mirroring how a
consumer ROUTING file would route the same paths in production.

* [jwbron/live-eval-ab-runner] review: carry agent-failure reasons into the A/B report

The acceptance runs surfaced a claim-validator failure that the
report could only name, not explain (perCase carried agent names
only, and the PerAgentReport.failed detail never reached the
markdown). perCase failedAgents entries are now '<agent>: <reason>',
so the next failure is diagnosable from the sticky comment alone.

* [jwbron/live-eval-ab-runner] review: close every A/B report with a per-row stability footer

The phase 4 acceptance pair measured which report rows a reader may
act on from one run: recall, verdict agreement, the regression lists,
and the adversarial gate reproduced exactly on the no-op control,
while judge quality and noise moved on jitter alone; and on the
weakened arm judge quality went UP 0.16 as recall fell 17 points
(fewer, surer comments each score better). The footer states this on
every report so nobody chases a judge-quality delta or reads one as
health.

* [jwbron/live-eval-ab-runner] review: identity short-circuit, gate-flip retry, drop-bucket taxonomy

Three instrument fixes from the eval-tuning memo, landed in the runner
so the whole stack above inherits them:

- Pre-flight identity short-circuit (memo item 1): byte-identical
  review.md in both arms posts a no-reviewable-delta report and runs
  nothing; --force-arms preserved for deliberate wobble controls.
- Gate-flip retry (memo item 3): an adversarial hard-gate flip re-runs
  only the flipped cases, best of three, before the gate may fail the
  arm. Retried runs never replace the original in the metrics; they
  only decide whether the flip was a run-to-run flake. Retry spend is
  recorded in the report.
- Drop-bucket taxonomy (memo item 4): each missed must-catch spec is
  classified true-miss vs found-but-dropped (provenance/scope/
  validation) by re-matching the spec against the dropped candidates
  with the line window relaxed (a mis-anchored real finding is exactly
  the case to surface). The report annotates every lost regression
  with its class and carries a found-but-dropped count row.

* [jwbron/live-eval-ab-runner] review: judge economics (Haiku pin, retry with backoff)

The judge grades one comment's prose in 512 tokens; every load-bearing
metric (recall, verdict agreement, regressions, adversarial gate) is
deterministic and never touches it, and the acceptance runs showed the
judge signal is not single-run-stable on any model (it moved 0.11 on
the byte-identical control and went UP on the weakened arm). Price
that signal accordingly: the pin moves from claude-opus-4-8 to
claude-haiku-4-5-20251001 (a fifth of the cost; dated snapshot so
week-over-week scores compare; both arms always share one judge, so
within-run deltas are unaffected). Supersedes operator direction 4,
which pinned Opus. Also adds retry with backoff (3 attempts, 429/408/
5xx/network only) to the one live judge seam; a single transient 500
previously wasted an entire weekly scoring pass.

* [jwbron/live-eval-ab-ci] review: document why the baseline is the base tip, not the merge-base

The workflow passes origin/<base_ref> (the base branch tip) while its
comments and the dispatch-input description said merge-base. The code
is the right side of that disagreement: pull_request runs check out
the PR merge commit, so the candidate tree already contains the base
tip; baselining on the same tip isolates the PR's own review.md delta,
where a merge-base baseline would fold upstream review.md movement
into the PR's measured numbers. Prose updated to match the code; no
behavior change.

* [jwbron/live-eval-ab-runner] review: type the judge response via the fetch signature (eslint no-undef)

* review: add the review-trial skill (live A/B phase 5) (#238)

Phase 5 of the live A/B eval plan (#232), independent of the eval-code stack (#233-#237): the `review-trial` Claude Code skill, packaging the Khan/webapp#40678 seeded-defect trial choreography so an operator can run one in an afternoon.

## What the skill does

Given a consumer repo, a seeded branch, a **human-authored** defect table, an arms list, and budget approval (projected up front from the trial's measured $7-10 per workflow-arm run, confirmed before any PR is created), it:

1. Creates one isolated PR per arm from the seeded branch, with per-arm trigger recipes (repo-default reviewer, pinned workflow ref, hosted `@claude review`) and two hard rules: distinct workflow names (same-named gh-aw workflows share a per-PR concurrency group and cancel each other; this silently ate a run in the original trial) and exactly one reviewer per PR (suppress the default via `skip-ai-review` on non-default arms).
2. Collects each run's review, artifacts (tolerating the known gh-aw staging-path artifact annotation), billed credits, and wall clock.
3. Optionally drives the lifecycle protocol (push 2 with fixes plus fresh seeds, push 3 all-fixed) identically across arms, scoring each push separately.
4. Scores defect by defect with the deterministic rule mirroring `eval/live-match.ts` (path + window + mechanism), manual judgment for leftovers marked as judged in the report, traps counted as correct suppression, and faithful reporting whichever arm it favors; output in the #40678 report shape.
5. Exports live-enabled corpus case skeletons, with an explicit sanitization gate for private-repo content landing in this public repo (the #235 lesson: structural rewrites, never copied code).
6. Cleans up: PRs closed with the report linked, trial branches deleted, no temporary workflow left behind.

What stays human is stated at the top of the skill: seeds and ground truth are operator-authored, always; the skill refuses to improvise a defect table.

## `.gitignore` change

`.claude` was ignored wholesale, which would have kept project skills untracked. Narrowed to `.claude/*` with a `!.claude/skills/` carve-out; verified `settings.json` and `worktrees/` remain ignored.

## Test plan:

Docs/tooling only (empty changeset); no code paths. `git check-ignore` verified the carve-out (SKILL.md tracked, `settings.json` and `worktrees` still ignored). The real acceptance is the plan's: re-running the #40678 trial via this skill reproduces its table in an afternoon, which needs a consumer-repo trial to exercise.


## Next steps (human)

1. Review the `.gitignore` change deliberately: `.claude` wholesale becomes `.claude/*` + `!.claude/skills/`. Verified locally that `settings.json` and `worktrees/` stay ignored, but this is a policy change about what agent tooling gets committed to the repo.
2. The skill's acceptance is operational, not CI: next time a trial is warranted (an architecture-bet change like the deterministic orchestrator, or pre-graduation ground-truthing), run it via this skill and check it reproduces the #40678 report shape in an afternoon. There is no cheaper honest validation.
3. Skim the per-arm trigger recipes against current consumer-repo reality (the `skip-ai-review` label convention, ready-for-review semantics); those conventions live outside this repo and can drift.

Author: jwbron

Reviewers: jeresig, github-actions[bot], somewhatabstract

Required Reviewers:

Approved By: jeresig, github-actions[bot]

Checks: ✅ 10 checks were successful

Pull Request URL: #238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants